feat(highlights): wire BibleReader to the highlights API (YPE-1034)#288
feat(highlights): wire BibleReader to the highlights API (YPE-1034)#288cameronapak wants to merge 13 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dark-launch flag Replace the localStorage highlight store (YPE-642 stand-in) with real API wiring per YPE-1034 ADR-001: highlights are server-only account data, deleted locally with no migration. - New internal useBibleReaderHighlights seam: fetches the current chapter's highlights via useHighlights (chapter USFM passage_id, keyed on version), renders through an in-memory optimistic overlay, reverts + console.error on write failure (toasts and 401/403 handling are PR 2) - Contiguous verse runs collapse to range USFMs on the wire (usfm-ranges.ts, mirroring the verse-share run-grouping idiom); colors sent as lowercase hex - Internal HIGHLIGHTS_LIVE dark-launch flag (off, not exported from the package entry) gates fetches, writes, and rendering; setHighlightsLive() is the test/Storybook-only override - Auth-gated: reads YouVersionAuthContext directly (now exported from hooks) so a missing auth provider degrades to signed-out instead of the useYVAuth throw; sign-out un-renders highlights immediately - better-result added to the ui package for error typing at the new seam's write boundary; core's throwing clients are untouched Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… responses Adversarial review of the highlights wiring surfaced one blocker and two races, all rooted in useApiData: - The fetch effect ignored enabled transitions: a hook that mounted disabled (auth still resolving) never fetched once enabled flipped true, so a signed-in reader rendered zero highlights until a write or navigation. enabled now rides alongside the caller-supplied deps. - Disabling kept the last response, so stale account data could render across sign-out or a host-controlled auth user switch. Disabling now clears data and error. - refetch-initiated requests escaped cancellation: a stale refetch for a previous chapter resolving late could clobber the new chapter's data. All requests now go through a monotonic sequence; only the latest-issued request may commit state. In the BibleReader seam hook: - Successfully settled writes now hand their verses to a confirmed set whose overlay entries are dropped when the post-write refetch lands, so server truth wins again instead of the optimistic entry masking later remote changes until navigation. - Documented the two remaining apply/remove concurrency windows (in-flight POST vs DELETE ordering, snapshot revert vs concurrent write) at the write boundary; a real operation queue is deliberately PR 2. New tests: useApiData enabled transitions and stale-response handling, and a seam-hook integration test through the real useHighlights/useApiData path (module-level mocks had hidden the enabled-flip bug). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Turn a color tap without a session or the `highlights` permission into an applied highlight across the two auth paths, behind the internal HIGHLIGHTS_LIVE flag. Core: DataExchangeClient (POST /data-exchange/token, Zod-validated), the hosted-grant URL builder + callback parser/handler, and an optimistic permission cache on YouVersionPlatformConfiguration seeded from `granted_permissions` on the sign-in and data-exchange callbacks (server 401/403 invalidates it). SignInWithYouVersionResult gains `permissions`. Hooks: useHighlightAuthActions exposes one-fell-swoop sign-in (requesting `highlights`), the just-in-time data-exchange redirect, permission reads/invalidation, and the data-exchange return handler. UI: useBibleReaderHighlights runs the state machine — pending highlights persist to sessionStorage (~10-min expiry) across the redirect round-trip and apply on a granted return; a permission confirm dialog (copy matched to the native SDK, en/fr/es) gates the grant. Write failures route by status (401/403 re-prompts and keeps pending; 5xx/network reverts and discards). Apply/remove writes are serialized through a FIFO queue with per-verse ownership, closing the two concurrency windows PR1 documented. Copy/share-only behavior with no auth provider is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on hydrates The resume effect consumed the data-exchange return status into a local, but bailed on `!isAuthenticated` first. The shipped YouVersionAuthProvider hydrates userInfo asynchronously, so the first effect run after a redirect return is always unauthenticated: the status was consumed and lost, and when the session flipped the cancel/failure branch was skipped — the pending highlight survived a decline and the just-declined dialog re-opened. Discard the pending highlight at consume time for any non-granted return, before the auth gate: it runs exactly once and needs no session (a decline kills the intent regardless of who signs in). Tests: the cancel-return test previously set signedIn=true synchronously — a timing the real provider never produces — which hid the bug. It now mounts signed out and flips the session after mount (failed against the old code, passes now), plus a failure-status variant with the same timing and the granted-return test aligned for realism. Also document the three review-deferred follow-ups (expired-token 401 misrouting, resume-write failure handling, remove-failure re-prompt) at their code sites and in the changeset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…watch Render the server's recent-colors list in BibleReader's verse action popover, and swap the active/remove swatch's X for a checkmark to match iOS (platform-sdk-swift #179). Still behind the internal HIGHLIGHTS_LIVE flag (YPE-1034 PR3). - useBibleReaderHighlights fetches getRecentColors once the feature is live (flag on + auth provider + signed in) and exposes recentColors; it resets to null on sign-out / flag-off and on fetch failure so the row falls back to the default palette. - VerseActionPopover takes an optional recentColors prop: normalized (lowercase, strip leading #, drop non-6-hex), deduped first-occurrence-wins, never reordered (server order is truth), and horizontally scrollable on overflow. Falls back to HIGHLIGHT_COLORS when recents are absent/empty. Active colors outside the palette still get a remove circle. - HighlightColor widened from the 5-literal union to string (loosening, non-breaking); HIGHLIGHT_COLORS still exported as the default palette. - New icons/check; ADR-005 as-built notes updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups on the recent-colors popover change: - The row's new `overflow-x: auto` forces overflow-y out of `visible` (CSS spec), clipping the swatches' focus-visible ring (ring-2 + offset-2, ~4px overpaint) and hover scale-110 — including for prod users with HIGHLIGHTS_LIVE off, since the popover itself is not flag-gated. Add 6px padding with compensating negative margin so the overpaint stays inside the scroll box without changing layout, and a regression test guarding the padding/overflow pairing. - Rework the primary recent-colors integration test to the production auth timing: mount signed out (as YouVersionAuthProvider hydrates async), assert no fetch, then flip signed-in and assert the fetch fires — the sync signed-in mount shape both prior PRs were dinged for. - Document the deferred account-switch-without-sign-out staleness at the fetch effect (recents keyed on `live` only, not user identity). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ui: default button variant now bg-primary/text-primary-foreground (was white-on-white in light theme, making the permission dialog's Continue button invisible). YouVersionAuthButton pins bg-background so its brand surface is unchanged. - ui: retire the optimistic overlay only once a fetch reflects the write, so read-after-write lag no longer flickers a highlight out/back (apply) or resurrects a removed one (remove). - hooks/ui: stop refetching per write; the seam hook coalesces to one refetch per settled batch. - core/ui: send one DELETE per verse instead of a range (range delete is unsupported server-side); docstring updated to stop claiming it. - core styles: fade highlight fills in/out (~250ms, reduced-motion aware). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re too An adversarial review caught a regression from the refetch coalescing: on ANY failure the whole batch's overlay reverted and no refetch fired, so sub-writes that succeeded server-side vanished from the UI (apply) or resurrected as ghosts (per-verse remove) until the next navigation/write. - runApply / runRemove / the resume-path write now track each sub-write's outcome: succeeded runs/verses register for reconciliation (overlay holds until a fetch reflects them), only truly-failed verses revert. - Exactly one refetch per batch, success or failure, restoring the self-correction the per-write auto-refetch used to provide. - Integration tests for partial apply failure, partial remove failure (no ghosts), and all-fail; the overlay-retirement test now distinguishes retired from still-masking via a divergent server color. - Changeset notes the partial-failure semantics and the accepted concurrent-edit staleness trade-off (pending product sign-off). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: cf593b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
There's one more issue where the highlight after being deleted comes back for split seconds, like a vapor, and then disappears. Isn't life like a vapor? |
|
One potential issue is... we want to disable highlights action bar and actions by default, unless the user specifies they want it. The highlights action bar right now goes as such: If |
…view fixes - Rewrite the highlights flow as an xstate v5 machine (bible-reader-highlights-machine.ts); useBibleReaderHighlights is now a thin adapter. Statechart doc in docs/highlight-flow-statechart.md. - Add SignInWithYouVersion dialog (Swift sheet ported as a dialog) shown on a signed-out color tap, with all 14 canonical i18n keys (en/es/fr) and new appName/signInPromptMessage config. - Fix the "vapor" bug: remove-overlay entries no longer retire on reflection, so a stale replica read can't resurrect a deleted highlight. - Flag-off now hides the highlight UI entirely (color row + remove circles). - Scope the granted-permissions cache to the signed-in user (Greptile P1). - Data-exchange callback cleanup now strips only its own params (Greptile P2). - Remove-failure 401/403 no longer re-prompts the permission dialog. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All three items addressed in def70d3, plus the flow is now an xstate v5 statechart per our discussion (full rewrite — nothing is live yet, so this was the moment to structure it right). Vapor bug — root cause confirmed with a failing-first reproduction: the reconcile step retired a remove-overlay entry as soon as any fetch reflected the deletion, so a later stale read-replica response containing the highlight repainted it for a frame. Fix: remove-overlay entries never retire on reflection — a removed verse stays suppressed until scope change, sign-out, or a newer write re-claims it. Same accepted trade-off the PR already documents. "You are a mist that appears for a little while and then vanishes" — the highlight, however, now stays gone. Sign-in dialog — ported from Flag-off behavior — confirmed and implemented: with The statechart is in |
A pending highlight resumed after sign-in/data-exchange now routes write failures through the same handling as a user-initiated apply: 401/403 invalidates the permission cache, re-stashes the pending highlight (in its own scope), and re-opens the permission dialog instead of silently dropping the user's tap. No unattended redirect loop: the dialog requires a click. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
For the record on Greptile's last finding ("Pending Intent Still Drops", now resolved): the 5xx-on-resumed-write behavior is intentional and documented in code as of cf593b1. Transient failures consume the highlight intent uniformly with user-initiated applies — after the redirect round-trip the user is signed in with the permission, so a re-tap just works, and the failure surfaces via the snackbar (YPE-3873). Re-stashing would auto-apply the highlight on a later mount within the 10-min TTL with no user action, which is a worse surprise than asking for one more tap. Flagging so product can override if silent-retry semantics are preferred. |

Summary
BibleReader highlights read/write the real
/v1/highlightsAPI — now orchestrated by an explicit xstate v5 statechart — with a Sign in with YouVersion dialog, a just-in-time permission flow, recent colors, and staging-hardening fixes, all dark-launched behind the internalHIGHLIGHTS_LIVEflag (off; flip tracked in YPE-3874). Consolidates the YPE-1034 stack (#283, #285, #286, #287) into one reviewable PR, rebased onto main.Start here: the flow is a state machine now —
packages/ui/src/components/bible-reader-highlights-machine.ts, with a mermaid statechart in docs/highlight-flow-statechart.md. Paste the machine into the Stately visualizer to explore it interactively.Changes
useBibleReaderHighlightsis a thin adapter. All previously-shipped invariants preserved (writes serialized FIFO, per-verse ownership tokens, per-sub-write settlement, apply collapses to range USFMs / remove DELETEs per verse, one refetch per settled batch, scope change drops the overlay).SignInWithYouVersionView(as a dialog, per web convention): INTRODUCING eyebrow, platform logo, optional integrator pitch fromYouVersionPlatformConfiguration.signInPromptMessage, body withYouVersionPlatformConfiguration.appName, Yes Please / No Thanks. Confirm launches OAuth requestinghighlights; decline keeps the verse selection. All 14 canonical string keys landed in en/es/fr (JIT + sign-out dialog copy included for future use).HIGHLIGHTS_LIVEoff the color row and remove circles don't render at all (Copy/Share stay). Previously the row rendered with inert taps.GET /v1/highlights/recent-colors, fallback to the 5-color palette); active swatches show a checkmark.{userId, permissions}; user B can never read user A's grants, and a callback that doesn't echogranted_permissionscan't resurrect stale ones.data_exchange_status/granted_permissionsare stripped; host app query params and the hash survive.useApiDatafetches whenenabledflips false→true, clears data on disable, and drops stale responses via latest-wins sequencing.Flow
flowchart LR A[Tap color] --> B{Signed in +<br>highlights permission?} B -- yes --> C[Optimistic overlay + POST] B -- no session --> D[Sign-in dialog] -- Yes Please --> E[OAuth requesting highlights] B -- no permission --> F[JIT confirm dialog] -- Continue --> G[Data-exchange grant] D -- No Thanks --> H[Discard pending, keep selection] F -- Cancel --> H E --> I[Pending highlight resumes] G --> I I --> CFull statechart: docs/highlight-flow-statechart.md
Breaking / Migration
Local-only highlights are gone: the localStorage store is deleted without migration per the server-only ADR — highlights are account data and un-render on sign-out.
Test plan
pnpm test: 893 passing (core 345, hooks 285, ui 263),pnpm typecheckandpnpm lintcleanHIGHLIGHTS_LIVEflips: one live data-exchange round-trip on staging — callback params (data_exchange_status,granted_permissions) are assumed from spec; handling is isolated inbuildDataExchangeUrl/parseDataExchangeCallbackReplaces the stacked drafts #283, #285, #286, #287.
🤖 Generated with Claude Code
Greptile Summary
This PR wires BibleReader highlights to the live highlights API behind a dark-launch flag. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Comments Outside Diff (2)
packages/ui/src/components/use-bible-reader-highlights.ts, line 3837 (link)When the resumed highlight write fails after a sign-in or data-exchange return, this path has already deleted the pending highlight. A 401/403 or transient write failure then only logs and reverts, so the tap that started the auth flow is lost instead of being kept and re-prompted like the normal apply path.
Prompt To Fix With AI
packages/ui/src/components/use-bible-reader-highlights.ts, line 3436-3437 (link)If
GET /v1/highlightsever returns a range passage such asJHN.3.16-18,parseIntmaps it only to verse 16. The trailing verses are omitted fromhighlightedVerses, and the reconcile path using the same parsing can leave overlay entries for verses 17-18 pinned until navigation.Prompt To Fix With AI
Reviews (4): Last reviewed commit: "docs(highlights): document intentional 5..." | Re-trigger Greptile
Context used (8)